Conversation
Upgrade io v0.2.0→v0.4.1, log v0.0.4→v0.1.2 (forge path → dappco.re). Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
…re primitives Replaced fmt, strings, sort, os, io, sync, encoding/json, path/filepath, errors, log, reflect with core.Sprintf, core.E, core.Contains, core.Trim, core.Split, core.Join, core.JoinPath, slices.Sort, c.Fs(), c.Lock(), core.JSONMarshal, core.ReadAll and other CoreGO v0.8.0 primitives. Framework boundary exceptions preserved where stdlib types are required by external interfaces (Gin, net/http, CGo, Wails, bubbletea). Co-Authored-By: Virgil <virgil@lethean.io>
Spark draft + opus cleanup. Two real bugs fixed:
- collectJSONKeys("") built listing path as baseDir+"/" which the
MockMedium.List doubled to "//", silently returning zero matches.
Every pattern-based op iterated an empty key set
- matchKeyPattern used path.Match where * does not cross /. Per RFC
§12.4 "dns/*" must match "dns/example.com/A" recursively. Rewrote
to treat trailing /* as prefix semantics (any depth) plus per-
segment glob for middle wildcards like "dns/charon.*"
Plus AX cleanup of spark's stdlib reintroductions (strings, sort,
path) — swapped to core.* primitives (Contains, HasPrefix, HasSuffix,
TrimSuffix, Split, SplitN, Trim, JoinPath) and slices.Sort.
Feature work preserved:
- scoped caches, invalidation triggers, HTTP CacheStorage, binary
data support, per-key TTL
- ensureSafeCacheName extracted to DRY CacheStorage.Open/Delete
- All 20 tests pass
Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
Minimal driver exercises cache.New with coreio.NewMockMedium (matching the cache_test.go idiom), Set + Get round-trip on a map payload, exits 0 on success and 1-5 at each failure step for diagnostic precision. Taskfile default task: build the driver, run it, clean up the binary. dir: ../../.. so task -d tests/cli/cache runs at module root. The driver's package main is correctly picked up by go test ./... as [no test files] and does not break the existing test suite. Closes tasks.lthn.sh/view.php?id=382 Signed-off-by: Snider <snider@host.uk.com>
go.mod: dappco.re/go/core/io → dappco.re/go/io. cache.go + cache_test.go + tests/cli/cache/main.go imports rewritten. `go test -mod=readonly ./...` passes. Closes tasks.lthn.sh/view.php?id=649 Co-authored-by: Codex <noreply@openai.com>
Removed local `replace dappco.re/go/io => ../go-io` — upstream module now resolvable via proper registry (go-io migrated via #629). Replace directive was leftover dev-mode bypass. Closes tasks.lthn.sh/view.php?id=787 Co-authored-by: Codex <noreply@openai.com>
Pre-migration v0.x.y tag versions are no longer publishable; v0.8.0-alpha.1 is the canonical target across the dappco.re/go/* namespace per 2026-04-24 Lethean release-gate sweep. Co-Authored-By: Athena <athena@lthn.ai> Co-Authored-By: Virgil <virgil@lethean.io>
…ink escape (Cerberus) Two reachable findings closed via Cerberus-supervised codex audit (Mantis #923): 1. Untrusted-key DoS via Invalidate (medium): Invalidate accepted callback-returned glob patterns without bounds, which then drove keysByPattern through a full key listing. New ensureSafePattern enforces a 4096-byte cap before listing. Repro: TestCache_Invalidate_UntrustedPatternLength_Bad 2. Path traversal via symlink escape (high): ensureSafeKey rejects byte-pattern traversal (.., null byte, leading /, control chars) but did not catch runtime-state symlink-following — a symlinked subdirectory inside baseDir could redirect an otherwise safe key outside the cache root. New ensureNoSymlinkPath walks each path segment via os.Lstat and rejects symlink components. Repro: TestCache_Path_PathTraversalSymlink_Bad Section 3 (TOCTOU on Cache.mu / Invalidate-while-OnInvalidate) is documented in threats.md as TBD — codex budget exhausted before reaching it. Filed as a separate follow-up ticket per scope-expansion discipline. Verification: go vet clean; go test -race -count=1 passes (1.341s). Workspace mode required (pre-existing task #28 forge dep break affects GOWORK=off; not introduced by this commit). Found-by: Cerberus (mechanism-tier audit) Audit-by: codex (supervised by Cerberus) Closes tasks.lthn.sh/view.php?id=923 Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Cerberus <cerberus@lthn.ai> Co-Authored-By: Virgil <virgil@lethean.io>
Three threat-classes audited: 1. Untrusted-key DoS — oversized keys bounded by validateKey path. Test added. 2. Path traversal — ScopedCache uses SHA-256 of origin (raw origins never embedded). HTTPCache uses SHA-256 of method+URL as storage key. Tests added. 3. Eviction TOCTOU — covered by sibling #924; this audit confirms boundaries. threats.md landed with question/finding/severity/line citations across all 3 sections. No new fixes required — existing defences hold; adds regression coverage. Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=923
Three TOCTOU sub-sections audited:
3.1 Invalidate map walk vs OnInvalidate registration — None (snapshot
semantics under Cache.mu, callbacks copied under RLock then released).
3.2 TTL expiry race on Get — None (metadata-first expiry check returns
not-found before unmarshalling cached data).
3.3 Get-then-Set caller-site TOCTOU — Fixed. Added entryMu to serialize
cache entry I/O separately from Cache.mu (which still guards
invalidation callbacks). Get/GetBinary RLock entryMu; Set/SetBinary
Lock entryMu across path resolve + rollback snapshot + write. Delete
paths also serialized.
Race-stress test added (TestCache_ThreatTOCTOU_GetThenSetSerializesEntryWrites)
demonstrates pre-fix race + post-fix safety.
Co-authored-by: Codex <noreply@openai.com>
Closes tasks.lthn.sh/view.php?id=924
- Removed encoding/json from cache.go - Replaced json.RawMessage with local raw JSON type - JSON marshalling moved through core.JSONMarshal - Pretty-printed cache metadata format preserved - // Note: AX-6 annotations added to retained banned imports with no usable core equivalent in pinned dep set Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=650
- errors.New → core.E - strings.Contains → core.Contains (strings import removed) - os.Getwd / os.Stat → t.TempDir / core.Env / medium.Stat patterns - crypto/sha256, encoding/base64, encoding/hex, encoding/json, io/fs, test-only os.Symlink — annotated // Note: AX-6 — test-only Race suite (with modfile workaround) PASS. Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=651
Removed strings import. Replaced strings.Builder → core.NewBuilder and strings.TrimPrefix → core.TrimPrefix. Co-authored-by: Codex <noreply@openai.com>
…) (#924 Cerberus) Filled Section 3 of threats.md with Question/Finding/Severity/Repro test/Fix for all three concerns: 1. Invalidate vs OnInvalidate concurrent registration 2. TTL expiry race on concurrent Get 3. Get-then-Set caller-side TOCTOU Added threat-named race-stress tests in cache_test.go covering invalidation snapshot, concurrent expired Get, and get-then-set race probe. Tests pass under -race -count=10. No cache.go hardening needed — analysis showed existing locking is correct. Closes tasks.lthn.sh/view.php?id=924 Co-authored-by: Codex <noreply@openai.com>
… (#380) Removed net/url; encodePathSegment uses core.URLPathEscape. Replaced os.IsNotExist with core.Is(err, fs.ErrNotExist) and os.ModeSymlink with fs.ModeSymlink. Retained io/fs and os with AX-6 annotations: fs.ErrNotExist, fs.ModeSymlink (sentinel access); os.Lstat (no-follow), os.Getwd (dynamic). Closes tasks.lthn.sh/view.php?id=380 Co-authored-by: Codex <noreply@openai.com>
#379:
- Removed crypto/sha256, encoding/base64, encoding/hex imports from cache.go
- Namespace/request hashing → core.SHA256Hex
- JSON entry serialization → core.JSONMarshalString
- Legacy raw URL base64 decoding kept inline without encoding/base64 import
#381:
- Removed sync import (strings was already absent)
- Removed backing-store entry mutexes; added caller-responsibility comment
- Internal invalidation + HTTP cache registries now use Core named locks
via runtime.Lock("cache") / runtime.Lock("cache-storage")
Verification: go test ./..., go vet ./..., race tests pass. One unrelated
stale TOCTOU test expects RFC §2-removed write mutex, predates this batch.
Co-authored-by: Codex <noreply@openai.com>
Closes tasks.lthn.sh/view.php?id=379
Closes tasks.lthn.sh/view.php?id=381
sync.RWMutex now serialises Get/Set/Delete/DeleteMany/binary entry ops and invalidation key enumeration. The previous AX-6 sweep at #379/#381 removed the mutex; concurrent writes corrupted the underlying map and re-introduced the TOCTOU race that #924 originally fixed. sync import is structural concurrency primitive (AX-6 permitted via established convention). Race regressions: 100 concurrent Sets on distinct keys, 100 concurrent Sets on same key, mixed Get/Set/Delete on same key — all -race clean. Co-authored-by: Codex <noreply@openai.com> Closes tasks.lthn.sh/view.php?id=985
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Routine dev→main sync that updates the go-cache module identity and documentation while introducing significant cache feature work (binary payload support, scoped caches, HTTP cache storage), plus security/threat-model artifacts and expanded tests.
Changes:
- Renames the module path to
dappco.re/go/cacheand updates docs accordingly. - Adds security/threat-model documentation and broadens the test suite (including threat/regression tests).
- Introduces additional cache capabilities (binary sidecars, invalidation patterns, scoped caches, HTTP cache storage) and a small CLI driver under
tests/cli.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
cache.go |
Major implementation expansion (validation, scoped caches, invalidation, binary caching, HTTP cache storage). |
cache_test.go |
Large test suite expansion covering new behaviors and threat/regression cases. |
go.mod |
Module path rename and dependency updates. |
go.sum |
Dependency checksum updates (currently appears incomplete). |
docs/index.md |
Updates module path and examples; dependency table updated but currently inconsistent with go.mod. |
docs/development.md |
Updates example imports; currently may not match new go-io module path. |
docs/architecture.md |
Adds sections on scoped caches/invalidation/HTTP cache storage; contains a hash algorithm mismatch. |
docs/api-contract.md |
Updates exported API inventory; currently mismatches Entry.Data type vs code. |
docs/security-attack-vector-mapping.md |
Updates scope module path reference. |
threats.md |
Adds threat-model audit notes and references to tests/fixes. |
tests/cli/cache/main.go |
Adds a simple CLI driver to exercise cache round-trip. |
tests/cli/cache/Taskfile.yaml |
Adds a task to build/run the CLI driver. |
CLAUDE.md |
Updates module path reference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "forge.lthn.ai/core/go-cache" | ||
| "forge.lthn.ai/core/go-io/store" | ||
| "dappco.re/go/cache" | ||
| "dappco.re/go/core/io/store" |
There was a problem hiding this comment.
The example imports dappco.re/go/core/io/store, but this module now depends on dappco.re/go/io (and cache.go imports dappco.re/go/io). If the store backend lives under the new module, this import path should likely be dappco.re/go/io/store; otherwise, consider adding the correct dependency (or clarifying why the core/io store path is still required).
| "dappco.re/go/core/io/store" | |
| "dappco.re/go/io/store" |
| if !result.OK { | ||
| return "", result.Value.(error) | ||
| } | ||
| return indentJSON([]byte(core.JSONMarshalString(value))), nil |
There was a problem hiding this comment.
marshalPrettyJSON marshals the value twice (core.JSONMarshal for error checking, then core.JSONMarshalString again for the actual bytes). This adds avoidable CPU/allocs on every cache write. Consider using the already-produced result.Value.([]byte) (or a single marshal path) and indenting that buffer instead.
| return indentJSON([]byte(core.JSONMarshalString(value))), nil | |
| return indentJSON(result.Value.([]byte)), nil |
| } | ||
|
|
||
| baseDir := absolutePath(cache.baseDir) | ||
| binaryPath := absolutePath(core.JoinPath(baseDir, key+".bin")) |
There was a problem hiding this comment.
entryPaths reuses Path(key) (which enforces the no-symlink traversal check) for the .json file, but constructs the .bin sidecar path without any ensureNoSymlinkPath validation. On filesystem-backed media, an attacker could potentially pre-create <key>.bin as a symlink and have SetBinary/GetBinary follow it outside baseDir. Consider applying the same ensureNoSymlinkPath(baseDir, binaryPath) check before returning binaryPath.
| binaryPath := absolutePath(core.JoinPath(baseDir, key+".bin")) | |
| binaryPath := absolutePath(core.JoinPath(baseDir, key+".bin")) | |
| if err := ensureNoSymlinkPath(baseDir, binaryPath); err != nil { | |
| return "", "", core.E("cache.entryPaths", "invalid cache key: symlink escape attempt", err) | |
| } |
| | Module | Version | Role | | ||
| |-------------------------------|---------|---------------------------------------------| | ||
| | `forge.lthn.ai/core/go-io` | v0.0.3 | Storage abstraction (`Medium` interface) | | ||
| | `forge.lthn.ai/core/go-log` | v0.0.1 | Structured logging (indirect, via `go-io`) | | ||
| | `dappco.re/go/core/io` | v0.4.1 | Storage abstraction (`Medium` interface) | | ||
|
|
There was a problem hiding this comment.
The Dependencies table lists dappco.re/go/core/io v0.4.1, but go.mod now requires dappco.re/go/io v0.8.0-alpha.1. Update this table (and any surrounding text that mentions go-io) to match the actual module path and version used by the package.
| ## Scoped Caches | ||
|
|
||
| `Scoped(origin)` creates a lightweight wrapper around the parent cache. The | ||
| wrapper hashes the origin with SHA-1 and uses the result as a fixed namespace |
There was a problem hiding this comment.
This section says scoped origins are hashed with SHA-1, but scopePrefix() in cache.go uses SHA-256 (core.SHA256Hex). Update the documentation to reflect the actual hash algorithm so consumers don’t rely on the wrong key derivation.
| wrapper hashes the origin with SHA-1 and uses the result as a fixed namespace | |
| wrapper hashes the origin with SHA-256 and uses the result as a fixed namespace |
| type Entry struct { | ||
| Data rawJSON `json:"data"` | ||
| CachedAt time.Time `json:"cached_at"` | ||
| ExpiresAt time.Time `json:"expires_at"` | ||
| } |
There was a problem hiding this comment.
Entry.Data is an exported field but its type is the unexported rawJSON. This makes it impossible for external packages (including cache_test which is in package cache_test) to construct cache.Entry values with Data set (e.g. Data: []byte("...") won’t compile). Consider using encoding/json.RawMessage (it already has the same marshal/unmarshal behavior) or exporting the type / changing the field type to []byte so external users/tests can build.
| | `GitHubRepoKey` | `func GitHubRepoKey(org, repo string) string` | `dappco.re/go/core/cache` | Returns the cache key for a specific repo's metadata. | yes | no | | ||
| | `DefaultTTL` | `const DefaultTTL = 1 * time.Hour` | `dappco.re/go/cache` | Default cache expiry time. | no | no | | ||
| | `Cache` | `type Cache struct { /* unexported fields */ }` | `dappco.re/go/cache` | File-based cache handle. | yes | no | | ||
| | `Entry` | `type Entry struct { Data json.RawMessage; CachedAt time.Time; ExpiresAt time.Time }` | `dappco.re/go/cache` | Cached item envelope with payload and timestamps. | no | no | |
There was a problem hiding this comment.
The API contract lists Entry as Data json.RawMessage, but cache.go currently defines Entry.Data as the unexported rawJSON type. Please update the contract table to match the actual exported API (or, preferably, switch Entry.Data back to json.RawMessage to keep the field usable by external callers).
| | `Entry` | `type Entry struct { Data json.RawMessage; CachedAt time.Time; ExpiresAt time.Time }` | `dappco.re/go/cache` | Cached item envelope with payload and timestamps. | no | no | | |
| | `Entry` | `type Entry struct { Data rawJSON; CachedAt time.Time; ExpiresAt time.Time }` | `dappco.re/go/cache` | Cached item envelope with payload and timestamps. | no | no | |
Routine dev→main PR opened by Hephaestus PR-cadence task.
This PR exists to:
ahead_by: 76 commit(s) (per gh api compare)If CodeRabbit clears with no blocking comments, this PR is eligible for
gh pr merge --merge(real merge commit, no force-push). Conflicts and review feedback should be addressed on the dev branch before merge.Co-authored-by: Hephaestus hephaestus@cladius